feat: Highlight and prioritize code definitions in grep results#299
feat: Highlight and prioritize code definitions in grep results#299yashksaini-coder wants to merge 8 commits intodmtrKovalenko:mainfrom
Conversation
dmtrKovalenko
left a comment
dmtrKovalenko
left a comment
There was a problem hiding this comment.
Thank you for the contribution but I think we can make it even better
yashksaini-coder
force-pushed
the
feat/context-definition-detection
branch
3 times, most recently
from
1606258 to
5f722dc
Compare
yashksaini-coder
force-pushed
the
feat/context-definition-detection
branch
from
f87e12d to
43331dc
Compare
|
@dmtrKovalenko can we run the CI/CD checks. I've updated the branch to latest commit |
|
Thank you. If you don’t mind I’ll push to your PR with some changes I’d like to tune how this feature feels |
Sure why not works for me @dmtrKovalenko Would like to see your approach as well for improving the highlighting in grep |
|
Hi @dmtrKovalenko follownig up on my PR, have you made any changes ? also I see some skipped workflows are they manual based for maintainers or reviewers |
There was a problem hiding this comment.
Pull request overview
This PR exposes the core grep engine’s definition-classification signal to Neovim and uses it to visually mark definition matches and optionally prioritize them in results, controlled via a new grep.classify_definitions config toggle.
Changes:
- Added
grep.classify_definitions(defaultfalse) and threaded it through the Lua → Rust FFI → core grep options. - Exposed
is_definitionon each grep match item to Lua and rendered a[def]indicator in the grep UI. - When enabled, core grep post-processes results to prioritize definition matches.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lua/fff/grep/init.lua | Passes classify_definitions through to the Rust live_grep FFI call. |
| lua/fff/grep/grep_renderer.lua | Adds virtual-text [def] marker for definition matches. |
| lua/fff/conf.lua | Adds grep.classify_definitions to config typing + defaults. |
| crates/fff-nvim/src/lua_types.rs | Includes is_definition in Lua grep match items. |
| crates/fff-nvim/src/lib.rs | Adds classify_definitions as a new optional live_grep parameter and forwards into GrepSearchOptions. |
| crates/fff-core/src/grep.rs | Sorts matches to prioritize definitions when classification is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dmtrKovalenko I have addressed the concerns and issues raised by copilot review, looking forward to your review and feedback, Thanks |
|
I am sorry brother, I have been working on some additional changes because I really want this feature to shine and jumped away to the random markerting PRs i will revisit, push some changes and merge this PR in a few days |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
crates/fff-nvim/src/lib.rs:295
live_grep’s destructured argument tuple includestrim_whitespace, but the tuple type annotation below only has 9 elements (ending atOption<bool>forclassify_definitions). This will not compile; update the tuple type to include an additionalOption<bool>fortrim_whitespace(and keep the order aligned with the Lua call).
classify_definitions,
trim_whitespace,
): (
String,
Option<usize>,
Option<usize>,
Option<u64>,
Option<usize>,
Option<bool>,
Option<String>,
Option<u64>,
Option<bool>,
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // All matches for a given file must be contiguous (no interleaving). | ||
| if !result_defs.matches.is_empty() { | ||
| let mut last_file = result_defs.matches[0].file_index; | ||
| let mut seen_files = std::collections::HashSet::new(); | ||
| seen_files.insert(last_file); |
There was a problem hiding this comment.
The new test asserts ordering invariants but doesn’t assert that the result set actually contains at least one is_definition == true match (and at least one non-definition) to make the assertions meaningful. If definition classification regresses to always-false, this test can still pass vacuously; consider adding explicit assertions about the presence of both kinds of matches in result_defs.matches.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes:- #297
This PR adds definition detection to the grep engine.
Kept it focused on definitions only for now to avoid messing with the UI for context lines.
@dmtrKovalenko